-
Notifications
You must be signed in to change notification settings - Fork 51
refactor: update required resources treatment and use subclasses over mixins #184
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
johnnygreco
merged 10 commits into
main
from
johnny/refactor/remove-required-resources-from-metadata
Jan 9, 2026
Merged
refactor: update required resources treatment and use subclasses over mixins #184
johnnygreco
merged 10 commits into
main
from
johnny/refactor/remove-required-resources-from-metadata
Jan 9, 2026
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
johnnygreco
commented
Jan 7, 2026
johnnygreco
commented
Jan 7, 2026
src/data_designer/engine/column_generators/generators/llm_completion.py
Outdated
Show resolved
Hide resolved
d7d6e9c to
c4f717b
Compare
nabinchha
reviewed
Jan 8, 2026
src/data_designer/engine/column_generators/generators/llm_completion.py
Outdated
Show resolved
Hide resolved
src/data_designer/engine/dataset_builders/column_wise_builder.py
Outdated
Show resolved
Hide resolved
c4f717b to
1b18b73
Compare
johnnygreco
commented
Jan 9, 2026
nabinchha
reviewed
Jan 9, 2026
src/data_designer/engine/column_generators/utils/generator_classification.py
Show resolved
Hide resolved
src/data_designer/engine/dataset_builders/column_wise_builder.py
Outdated
Show resolved
Hide resolved
mikeknep
previously approved these changes
Jan 9, 2026
nabinchha
previously approved these changes
Jan 9, 2026
mikeknep
approved these changes
Jan 9, 2026
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The problem we are solving
ConfigurableTaskMetadatarequires you to specify the resources required to execute the task. In practice, however, all tasks always have access to all resources. While this issue isn’t a big deal, it is confusing for plugin builders who don’t have the above context.Changes
Remove
required_resourcesfrom metadataAlways assume all tasks have access to all resources.
Use subclasses (instead of mixins) to streamline development and simplify plugin development.
An important note is that I think we still need some what for a generator to specify its required resources. For example, say we want to filter plugin types to only grab the ones that need the model registry or just need the datastore. The solution implemented here is an abstract method called
get_required_resourcesthat must be implemented on generators. This effectively pushes this complication to a lower lever, where most developers won't need to worry about it. I'll highlight some places in the code to show what I mean.